refactor(integration): extract MCPIntegrator.install by abhinavgautam01 · Pull Request #1245 · microsoft/apm

abhinavgautam01 · 2026-05-10T15:47:39Z

Description

This PR applies strangler-fig stage 1 for MCP install orchestration: the implementation of MCPIntegrator.install is moved into run_mcp_install() in src/apm_cli/integration/mcp_integrator_install.py. MCPIntegrator.install remains a thin delegate so the public API and existing call sites stay the same.

Motivation: mcp_integrator.py is dominated by a very large install method; extracting it improves readability and makes later refactors safer without changing behavior.
Compatibility: Imports inside run_mcp_install() keep test patch targets working (e.g. apm_cli.integration.mcp_integrator._get_console, MCPIntegrator._install_for_runtime). _get_console stays imported on mcp_integrator with noqa so patches that target that module attribute keep working.

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

Tested locally
All existing tests pass
Added tests for new functionality (if applicable)
After uv sync --extra dev:

uv run pytest tests/unit/integration/test_mcp_integrator.py \
  tests/unit/test_mcp_integrator_characterisation.py \
  tests/unit/test_global_mcp_scope.py \
  tests/unit/test_transitive_mcp.py \
  tests/unit/test_mcp_overlays.py -q

Result: 265 passed.

…r_install Strangler-fig stage 1: move install orchestration into run_mcp_install() while keeping MCPIntegrator.install as the public entrypoint.

abhinavgautam01 · 2026-05-10T15:48:31Z

@microsoft-github-policy-service agree

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

…ct-mcp-integrator-install

github-actions · 2026-05-14T06:04:28Z

APM Review Panel: `ship_with_followups`

Strangler-fig extraction of MCPIntegrator.install into mcp_integrator_install.py reduces file size and opens the install pipeline to contributor-safe iteration -- no user-visible behavior change.

cc @danielmeppiel -- a fresh advisory pass is ready for your review.

This PR delivers the first half of a clean strangler-fig split: the ~500-line install body is now in its own module, the public API is fully preserved, and the thin delegate pattern is correctly wired. Strategically, this is the prerequisite move before the install pipeline can carry easy-first-issue labels or receive isolated unit tests -- the oss-growth-hacker framing is correct and worth amplifying post-merge. The devx-ux-expert confirms no public surface was altered, and supply-chain-security-expert finds all security controls preserved. Panel signals converge strongly: this is a clean, safe merge with a well-defined list of follow-ups.

The three findings that deserve the most post-merge attention are ordered by real-world blast radius. The cli-logging-expert identified two silent-failure gaps (lines 457-458 and 550-551) where per-server install errors are swallowed entirely in non-Rich environments -- users on constrained CI or headless hosts would see zero output on failure. This is a pre-existing exposure faithfully carried over, but the extraction is the right moment to close it. Alongside that, the STATUS_SYMBOLS bracket convention is broken in the Rich path (bare > instead of [>]) creating divergent output depending on Rich availability. The test-coverage-expert's evidence block is outcome: missing on an integration-with-fixtures surface tagged devx and portability-by-manifest: all integration tests patch the thin delegate, so run_mcp_install's ~500-line body is invisible at the integration tier. A single un-mocked integration test closes the gap. The python-architect's back-reference finding is real -- run_mcp_install calls 9 private MCPIntegrator statics, capping the isolation gain -- but this is an expected intermediate state for a strangler-fig and should be tracked as a follow-up extraction, not a blocker.

No meaningful dissent exists across the panel. The doc-writer's hardcoded line-number finding is a genuine rot risk worth a quick fix. No panelist found a correctness regression, behavior change, or security exposure introduced by this PR.

Dissent. All active panelists agree this is a safe merge. The python-architect notes the extraction is half-done (9 private back-references remain); the oss-growth-hacker and devx-ux-expert both confirm the public surface is intact. No conflict to arbitrate -- the back-reference finding is an acknowledged intermediate strangler-fig state, not an architectural disagreement.

Aligned with: OSS / Community-Driven (smaller focused modules lower contributor activation energy), Pragmatic as npm (pure refactor, zero behavior change), and Portable by Manifest (integration-tier coverage gap means silent drift in run_mcp_install would not be caught before a user hits it).

Growth signal. Strangler-fig refactors like this one are the unsexy prerequisite for a healthy contributor funnel. Once run_mcp_install() is fully isolated from MCPIntegrator private statics, the install pipeline becomes a safe sandbox where a contributor can add a new runtime handler without reading the entire class. The immediate follow-up after this lands is to open a tracking issue tagged good-first-issue pointing at the 9 remaining back-references as the next extraction target.

Panel summary

Persona	R	N	Takeaway
Python Architect	1	2	Strangler-fig shell is clean; install function back-references 9 private MCPIntegrator statics, capping test isolation and leaving the extraction half-done.
CLI Logging Expert	3	2	2 silent-failure gaps in non-Rich path (errors swallowed), STATUS_SYMBOLS bracket convention broken in Rich path, 1 missing progress fallback. No Unicode chars found.
DevX UX Expert	0	1	Clean extraction -- public API, all 12 args, error paths, and scope filtering fully preserved. One pre-existing Gemini cwd inconsistency noted for follow-up.
Supply Chain Security Expert	0	1	Pure refactor preserves all security controls; one nit on lazy back-import ordering contract that could silently break if module load order changes.
OSS Growth Hacker	0	2	Pure internal refactor; positive contributor-experience delta. No conversion surface touched. Two nits around discoverability signaling.
Doc Writer	1	1	Two docs carry hardcoded mcp_integrator.py line numbers that may have drifted after the extraction; no CHANGELOG entry needed for this internal refactor; new module docstrings are adequate.
Test Coverage Expert	1	0	Unit tests flow through run_mcp_install; all integration tests mock the thin delegate, leaving the ~500-line extracted body invisible at the integration tier -- one integration-with-fixtures test would close the gap.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

[CLI Logging Expert] Per-server install failure messages are silently swallowed in non-Rich environments (lines 457-458, 550-551 -- no else branch after elif console: error blocks). -- Silent failures in headless/CI environments are the highest-impact user-facing defect in this diff. An else: logger.error(f"{dep} -- failed for all runtimes") branch closes the gap with one line each.
[Test Coverage Expert] All integration tests patch MCPIntegrator.install at the delegate boundary, leaving run_mcp_install's ~500-line body invisible at the integration tier (outcome: missing). -- Silent drift in argument order, early-return logic, or exception propagation inside run_mcp_install would reach users before any test catches it. One un-mocked integration-with-fixtures test in tests/integration/test_mcp_integrator_install.py closes the gap.
[CLI Logging Expert] Rich path emits bare > + x symbols; STATUS_SYMBOLS convention requires bracket-enclosed [>] [+] [x]. Same event renders differently depending on Rich availability. -- Fix is mechanical: emit [cyan]\[>][/cyan] or route through STATUS_SYMBOLS dict.
[Python Architect] run_mcp_install back-references 9 private MCPIntegrator static methods, capping test isolation and leaving the strangler-fig half-done. -- Expected intermediate state; tracking it as a follow-up extraction issue is the only way the isolation gain gets realized.
[Doc Writer] Two docs cite hardcoded mcp_integrator.py line numbers (security-and-supply-chain.md:88, mcp-as-primitive.md:132) that may have drifted after the extraction. -- Replace with file-path-only citations or commit-SHA permalinks to prevent rot on the next refactor.

Architecture

classDiagram
    direction LR

    class MCPIntegrator {
        +install(mcp_deps, ...) int
        -_detect_runtimes(scripts) list
        -_gate_project_scoped_runtimes(...) list
        -_install_for_runtime(...) bool
        -_detect_mcp_config_drift(...) set
        -_append_drifted_to_install_list(...) None
        -_apply_overlay(...) None
        -_build_self_defined_info(dep) dict
        -_check_self_defined_servers_needing_installation(...) list
    }

    class run_mcp_install {
        <<ModuleFunction>>
        +run_mcp_install(mcp_deps, ...) int
    }

    class MCPServerOperations {
        +validate_servers_exist(names) tuple
        +check_servers_needing_installation(...) list
        +batch_fetch_server_info(servers) dict
    }

    class InstallScope {
        <<Enum>>
        USER
        PROJECT
    }

    MCPIntegrator ..> run_mcp_install : delegates install() to
    run_mcp_install ..> MCPIntegrator : calls 9 private statics
    run_mcp_install ..> MCPServerOperations : registry ops
    run_mcp_install ..> InstallScope : scope filtering

    class run_mcp_install:::touched
    class MCPIntegrator:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

flowchart TD
    A(["CLI: apm install / apm mcp install"]) --> B["MCPIntegrator.install()\nmcp_integrator.py"]
    B --> C["run_mcp_install()\nmcp_integrator_install.py"]
    C --> D{"runtime param?"}
    D -- yes --> E["target_runtimes = [runtime]"]
    D -- no --> F["load apm.yml + RuntimeManager\ndetect installed runtimes"]
    F --> G["MCPIntegrator._detect_runtimes(scripts)\nintersect installed with script runtimes"]
    G --> E
    E --> H["MCPIntegrator._gate_project_scoped_runtimes()"]
    H --> I{"registry_deps?"}
    I -- yes --> J["MCPServerOperations.validate_servers_exist()"]
    J --> K["MCPIntegrator._detect_mcp_config_drift()"]
    K --> L["MCPServerOperations.batch_fetch_server_info()"]
    L --> M["MCPIntegrator._install_for_runtime() per server per runtime"]
    I -- no --> N{"self_defined_deps?"}
    M --> N
    N -- yes --> O["MCPIntegrator._check_self_defined_servers_needing_installation()"]
    O --> P["MCPIntegrator._install_for_runtime()"]
    P --> Q["return configured_count"]
    N -- no --> Q

sequenceDiagram
    participant CLI
    participant MCPIntegrator
    participant run_mcp_install
    participant MCPServerOperations

    CLI->>MCPIntegrator: install(mcp_deps, ...)
    MCPIntegrator->>run_mcp_install: lazy import + delegate (all args forwarded)
    run_mcp_install->>MCPIntegrator: lazy import back (_detect_runtimes, _gate_project_scoped_runtimes, 7 more statics)
    run_mcp_install->>MCPServerOperations: validate_servers_exist(names)
    MCPServerOperations-->>run_mcp_install: valid_servers, invalid_servers
    run_mcp_install->>MCPIntegrator: _detect_mcp_config_drift(deps, stored)
    MCPIntegrator-->>run_mcp_install: drifted set
    run_mcp_install->>MCPServerOperations: batch_fetch_server_info(servers)
    MCPServerOperations-->>run_mcp_install: server_info_cache
    loop per server per runtime
        run_mcp_install->>MCPIntegrator: _install_for_runtime(rt, dep, ...)
        MCPIntegrator-->>run_mcp_install: bool
    end
    run_mcp_install-->>CLI: configured_count

Recommendation

Merge as-is. The extraction is architecturally correct, the public API is fully preserved, and no security or behavioral regression was found by any panelist. Open three follow-up issues immediately post-merge: (1) close the silent-failure logging gaps in non-Rich paths (cli-logging-expert, highest blast radius), (2) add one un-mocked integration-with-fixtures test for run_mcp_install (test-coverage-expert, closes the integration-tier coverage gap), and (3) track the 9 private back-references as the next strangler-fig extraction step (python-architect). The doc line-number rot fix is a fast PR that can land in parallel.

Full per-persona findings

Python Architect

[recommended] run_mcp_install calls 9 private MCPIntegrator static methods back into the host module at src/apm_cli/integration/mcp_integrator_install.py
The strangler-fig achieves file-size reduction but not isolation: unit tests for run_mcp_install must still patch MCPIntegrator private methods. A cleaner end-state moves those helpers into mcp_integrator_install.py (or a shared module) so the extracted module is self-contained. This is a follow-up, not a blocker.
[nit] _get_console imported via mcp_integrator re-export instead of direct apm_cli.utils.console import at src/apm_cli/integration/mcp_integrator_install.py:62
A direct import from apm_cli.utils.console removes the re-export indirection and the noqa annotation, making the dependency graph cleaner.
[nit] Lazy import of entire MCPIntegrator module just to access module-level _is_vscode_available at src/apm_cli/integration/mcp_integrator_install.py:60
_is_vscode_available is a module-level function, not a class method. It could be moved to utils/ or imported directly without pulling the MCPIntegrator class itself.

CLI Logging Expert

[recommended] Per-server failure messages have no logger fallback -- silent in non-Rich environments at src/apm_cli/integration/mcp_integrator_install.py
Lines 457-458 and 550-551 use elif console: console.print(...) error with no else branch. When Rich is unavailable, a per-server installation failure produces zero output.
Suggested: Add else: logger.error(f"{dep} -- failed for all runtimes") after each elif console: error block.
[recommended] Rich console path renders bare >, +, x -- deviates from STATUS_SYMBOLS bracket convention at src/apm_cli/integration/mcp_integrator_install.py
STATUS_SYMBOLS convention defines [>], [+], [x] as full bracket-enclosed ASCII symbols. Rich path emits [cyan]>[/cyan] (renders as bare >) while fallback logger path correctly uses bracket notation.
Suggested: Emit [cyan]\[>][/cyan] to preserve bracket notation, or use STATUS_SYMBOLS dict consistently.
[recommended] Per-server progress has no logger fallback -- silent in non-Rich environments at src/apm_cli/integration/mcp_integrator_install.py
if console: blocks for per-server progress (lines 422, 428, 513, 522) have no else branches. Non-Rich users see no per-server progress during install loop.
Suggested: Add else: logger.progress(f"{dep}: {action_text.lower()} for {', '.join(target_runtimes)}") after each console-only progress block.
[nit] Closing footer +- prefix is tree-drawing syntax, not STATUS_SYMBOLS at src/apm_cli/integration/mcp_integrator_install.py
Consider [*] Configured N servers using STATUS_SYMBOLS success symbol for the closing line, reserving +- for structural tree nodes only.
[nit] Verbose runtime-detection block is console-only with incomplete logger parallel at src/apm_cli/integration/mcp_integrator_install.py
Console block emits 4 lines; logger else block emits 2-3. Agent consumers miss the Runtime Detection label and Target line in verbose mode.

DevX UX Expert

[nit] Gemini runtime detection uses Path.cwd() instead of project_root_path at src/apm_cli/integration/mcp_integrator_install.py:174
Pre-existing in mcp_integrator.py, faithfully copied here. A future caller passing an explicit project_root will silently get wrong gemini detection. Worth a follow-up fix but not introduced by this PR.

Supply Chain Security Expert

[nit] Lazy circular import pulls private symbols from the delegating module at src/apm_cli/integration/mcp_integrator_install.py:60
run_mcp_install() lazily imports MCPIntegrator, _get_console, and _is_vscode_available back from mcp_integrator. Creates an implicit ordering contract invisible to future refactors. Consider importing _get_console directly from apm_cli.utils.console and moving _is_vscode_available to a shared utils module.

OSS Growth Hacker

[nit] New module is a contributor-experience win -- worth a one-liner in CHANGELOG at CHANGELOG.md
Extracting a 500-line method lowers activation energy for first-time contributors who want to extend the install pipeline.
[nit] Module name mcp_integrator_install.py is navigable but not self-documenting at src/apm_cli/integration/mcp_integrator_install.py:1
A one-line module docstring -- e.g. # Install pipeline for MCP packages. Entry point: run_mcp_install() -- would make the file self-describing on first open.

Auth Expert -- inactive

PR is a pure strangler-fig refactor of MCP install orchestration; no auth, token management, credential resolution, AuthResolver, or HTTP authorization surfaces are touched.

Doc Writer

[recommended] Hardcoded source line numbers in docs may have drifted after the strangler-fig extraction at docs/src/content/docs/enterprise/security-and-supply-chain.md:88
docs/enterprise/security-and-supply-chain.md cites mcp_integrator.py:205,219-221 and docs/producer/author-primitives/mcp-as-primitive.md cites mcp_integrator.py:124-145. Hardcoded line numbers in docs rot on every refactor.
Suggested: Remove line-number suffixes and cite only the file path, or replace with a GitHub permalink to a specific commit SHA. Apply the same fix in docs/src/content/docs/producer/author-primitives/mcp-as-primitive.md:132.
[nit] docs/enterprise/security.md prose references MCPIntegrator.install as a method call -- accurate but omits new module location at docs/src/content/docs/enterprise/security.md
Public API is preserved so no correctness regression. If the source path is ever updated in this page, note that the install logic now lives in mcp_integrator_install.py.

Test Coverage Expert

[recommended] Integration tests mock the thin delegate, leaving run_mcp_install invisible to integration-tier coverage
All integration tests (test_policy_install_e2e.py, test_selective_install_mcp.py, test_install_local_bundle_e2e.py, test_global_mcp_lockfile_e2e.py) patch MCPIntegrator.install at the method boundary. After the strangler-fig extraction, this patch short-circuits the thin delegate before it can call run_mcp_install(). Unit tests in test_transitive_mcp.py DO flow through run_mcp_install (they only mock _install_for_runtime), so there is unit-tier signal. But the install pipeline surface floor is integration-with-fixtures -- and that tier is now a gap.
Suggested: Add one integration-with-fixtures test in tests/integration/test_mcp_integrator_install.py that does NOT mock MCPIntegrator.install or run_mcp_install and exercises a real install invocation through the thin delegate.
Proof (missing at integration-with-fixtures): tests/integration/test_mcp_integrator_install.py::test_run_mcp_install_delegates_full_orchestration -- proves: The full install orchestration body in run_mcp_install is reachable and produces correct output via the thin delegate [devx, portability-by-manifest, secure-by-default]

_{This panel is advisory. It does not block merge. Re-apply the

panel-review label after addressing feedback to re-run.}

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

refactor(integration): extract MCPIntegrator.install #1245 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
#1245 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1245 · ● 3.3M · ◷

abhinavgautam01 · 2026-05-14T06:27:31Z

@danielmeppiel sorry for the extra pings earlier... i realized today that repeated review reminders can be counterproductive, lesson learned and i will avoid doing that going forward...

danielmeppiel

Please address all comments flagged "recommended" per persona in the panel review. Thank you!

… doc refs

…ct-mcp-integrator-install

refactor(integration): extract MCPIntegrator.install to mcp_integrato…

2201561

…r_install Strangler-fig stage 1: move install orchestration into run_mcp_install() while keeping MCPIntegrator.install as the public entrypoint.

abhinavgautam01 requested review from danielmeppiel and sergio-sisternes-epam as code owners May 10, 2026 15:47

Copilot AI review requested due to automatic review settings May 10, 2026 15:47

Copilot started reviewing on behalf of abhinavgautam01 May 10, 2026 15:48 View session

Copilot AI reviewed May 10, 2026

View reviewed changes

abhinavgautam01 requested a review from Copilot May 12, 2026 04:22

Copilot started reviewing on behalf of abhinavgautam01 May 12, 2026 04:22 View session

Copilot AI reviewed May 12, 2026

View reviewed changes

Comment thread src/apm_cli/integration/mcp_integrator_install.py Outdated

abhinavgautam01 added 2 commits May 12, 2026 10:31

docs: fix run_mcp_install docstring after extraction

dab43bd

Merge remote-tracking branch 'upstream/main' into refactor/1004-extra…

ce27745

…ct-mcp-integrator-install

sergio-sisternes-epam added panel-review Trigger the apm-review-panel gh-aw workflow status/triaged Initial agentic triage complete; pending maintainer ratification (silence = approval). labels May 13, 2026

abhinavgautam01 and others added 3 commits May 14, 2026 07:52

style: ruff format mcp_integrator_install

4f0bd51

Merge remote-tracking branch 'upstream/main' into refactor/1004-extra…

55c2eff

…ct-mcp-integrator-install

Merge branch 'main' into refactor/1004-extract-mcp-integrator-install

3585756

danielmeppiel added panel-review Trigger the apm-review-panel gh-aw workflow and removed panel-review Trigger the apm-review-panel gh-aw workflow labels May 14, 2026

github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 14, 2026

danielmeppiel requested changes May 14, 2026

View reviewed changes

abhinavgautam01 added 3 commits May 14, 2026 13:04

fix(mcp): non-Rich install logging; STATUS_SYMBOLS; integration test;…

f7c93c2

… doc refs

Merge remote-tracking branch 'upstream/main' into refactor/1004-extra…

49eb4c0

…ct-mcp-integrator-install

Merge remote-tracking branch 'upstream/main' into refactor/1004-extra…

195ec4a

…ct-mcp-integrator-install

Merge branch 'main' into refactor/1004-extract-mcp-integrator-install

253dee4

danielmeppiel approved these changes May 14, 2026

View reviewed changes

danielmeppiel enabled auto-merge May 14, 2026 15:32

danielmeppiel added this pull request to the merge queue May 14, 2026

Merged via the queue into microsoft:main with commit 08ab95b May 14, 2026
11 checks passed

danielmeppiel mentioned this pull request May 15, 2026

[docs] Update documentation for features from 2026-05-14 #1338

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(integration): extract MCPIntegrator.install#1245

refactor(integration): extract MCPIntegrator.install#1245
danielmeppiel merged 10 commits into
microsoft:mainfrom
abhinavgautam01:refactor/1004-extract-mcp-integrator-install

abhinavgautam01 commented May 10, 2026

Uh oh!

abhinavgautam01 commented May 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Auth Expert -- inactive

Doc Writer

Test Coverage Expert

Uh oh!

abhinavgautam01 commented May 14, 2026

Uh oh!

danielmeppiel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

abhinavgautam01 commented May 10, 2026

Description

Type of change

Testing

Uh oh!

abhinavgautam01 commented May 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026

APM Review Panel: ship_with_followups

Panel summary

Top 5 follow-ups

Architecture

Recommendation

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Auth Expert -- inactive

Doc Writer

Test Coverage Expert

Uh oh!

abhinavgautam01 commented May 14, 2026

Uh oh!

danielmeppiel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

APM Review Panel: `ship_with_followups`